[google_maps_flutter] Convert heatmap controller to Swift - #12713
[google_maps_flutter] Convert heatmap controller to Swift#12713stuartmorgan-g wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request converts the heatmap controller and data conversion logic from Objective-C to Swift across the iOS packages (sdk9, sdk10, and shared_code), removing several Objective-C files in favor of Swift implementations. The review feedback highlights critical safety issues, pointing out potential runtime crashes from unsafely unwrapping optional values like heatmap.gradient and heatmap.weightedData. Additionally, the reviewer suggests performance optimizations, such as avoiding unnecessary CLLocation heap allocations by passing CLLocationCoordinate2D directly to path-creation helpers, and refactoring redundant image loading logic in ImageUtils.swift.
| var gmsMapViewType: GMSMapViewType { | ||
| switch self { | ||
| case .none: return .none | ||
| case .normal: return .normal | ||
| case .satellite: return .satellite | ||
| case .terrain: return .terrain | ||
| case .hybrid: return .hybrid | ||
| @unknown default: return .normal | ||
| } |
There was a problem hiding this comment.
With this being a var, is it set once and then never changed? Is it possible for the map type to change?
There was a problem hiding this comment.
It's just a var because that's the syntax for a computed property; because there's no set defined for it, it's not actually assignable. FGMPlatformMapType is an enum, so an individual instance of it can't change value.
| self.heatmapTileLayer = tileLayer | ||
| self.mapView = mapView | ||
| super.init() | ||
| HeatmapController.update(tileLayer, from: heatmap, mapView: mapView) |
There was a problem hiding this comment.
Doesn't this need to pass self.tileLayer?
There was a problem hiding this comment.
self.heatmapTypeLayer and tileLayer are the same at this point, so it doesn't actually matter which is passed.
|
|
||
| var image: UIImage? | ||
|
|
||
| switch bitmap { |
There was a problem hiding this comment.
add back comment
| switch bitmap { | |
| // See comment in messages.dart for why this is so loosely typed. See also | |
| // https://github.com/flutter/flutter/issues/117819. | |
| switch bitmap { |
There was a problem hiding this comment.
Fixed. I caught that in the conversion utils, but missed it here.
(Also, good news: this problem is fixed in the next PR, since Swift Pigeon supports a limited form of class hierarchy for this use case.)
| case let bitmap as FGMPlatformBitmapBytes: | ||
| // Deprecated: This message handling for 'fromBytes' has been replaced by 'bytes'. | ||
| // Refer to the flutter google_maps_flutter_platform_interface package for details. | ||
| image = UIImage(data: bitmap.byteData.data, scale: screenScale) |
There was a problem hiding this comment.
Is the try catch not necessary here?
There was a problem hiding this comment.
You can't catch NSExceptions in Swift, so if UIImage's constructor can leak NSExceptions in current versions of iOS, Apple has done something very wrong 🙂
A Swift do/catch would catch Swift errors, but this constructor can't throw those because it's not marked as throws.
| } | ||
| case let bitmap as FGMPlatformBitmapBytesMap: | ||
| let bytes = bitmap.byteData | ||
| image = UIImage(data: bytes.data, scale: screenScale) |
|
Some tests are failing |
There was a problem hiding this comment.
Some tests are failing
Ha. This is a fun little reminder of why we should be moving to Swift, where we have strong uniform typing 🙂
FGMPlatformBitmap *placeholderImage =
[FGMPlatformBitmap makeWithBitmap:[FGMPlatformBitmapDefaultMarker makeWithHue:0]];let placeholderImage = FGMPlatformBitmap.make(
withBitmap: FGMPlatformBitmapDefaultMarker.make(withHue: 0))Looks the same, right? Only, because hue is a nullable double in the Pigeon definition, the Obj-C FGMPlatformBitmap has to declare it as an NSNumber*, not a double. So in the Obj-C, 0 was actually a misspelling of nil... which didn't matter, because they are both just integers.
But in Swift, 0 is an integer in an NSNumber context, so is equivalent to the Obj-C @(0), and since the 0 is an int, it gets encoded with the wrong type, and so explodes on the Dart side.
Fixed by correcting the spelling of nil. (The potential for this kind of problem is eliminated in the next PR, when all of these data classes become Swift.)
| var gmsMapViewType: GMSMapViewType { | ||
| switch self { | ||
| case .none: return .none | ||
| case .normal: return .normal | ||
| case .satellite: return .satellite | ||
| case .terrain: return .terrain | ||
| case .hybrid: return .hybrid | ||
| @unknown default: return .normal | ||
| } |
There was a problem hiding this comment.
It's just a var because that's the syntax for a computed property; because there's no set defined for it, it's not actually assignable. FGMPlatformMapType is an enum, so an individual instance of it can't change value.
| self.heatmapTileLayer = tileLayer | ||
| self.mapView = mapView | ||
| super.init() | ||
| HeatmapController.update(tileLayer, from: heatmap, mapView: mapView) |
There was a problem hiding this comment.
self.heatmapTypeLayer and tileLayer are the same at this point, so it doesn't actually matter which is passed.
|
|
||
| var image: UIImage? | ||
|
|
||
| switch bitmap { |
There was a problem hiding this comment.
Fixed. I caught that in the conversion utils, but missed it here.
(Also, good news: this problem is fixed in the next PR, since Swift Pigeon supports a limited form of class hierarchy for this use case.)
| case let bitmap as FGMPlatformBitmapBytes: | ||
| // Deprecated: This message handling for 'fromBytes' has been replaced by 'bytes'. | ||
| // Refer to the flutter google_maps_flutter_platform_interface package for details. | ||
| image = UIImage(data: bitmap.byteData.data, scale: screenScale) |
There was a problem hiding this comment.
You can't catch NSExceptions in Swift, so if UIImage's constructor can leak NSExceptions in current versions of iOS, Apple has done something very wrong 🙂
A Swift do/catch would catch Swift errors, but this constructor can't throw those because it's not marked as throws.
| } | ||
| case let bitmap as FGMPlatformBitmapBytesMap: | ||
| let bytes = bitmap.byteData | ||
| image = UIImage(data: bytes.data, scale: screenScale) |
This converts HeatmapController and the remaining utility functions to Swift in the
_sdk*packages.The final remaining Obj-C code will be migrated in a follow-up PRs.
The conversion process was:
The conversion code utils have much more change than previous PRs, since the direct conversion code felt very non-idiomatic in Swift. Almost all of the free functions for Pigeon<->Maps SDK type conversions were converted to extensions on the Pigeon types:
make(from:)toMapsSDKClassName()method on the Pigeon type (I'm not sold on that naming pattern; alternate suggestions welcome)mapthat conversion function, sincemapis a simple and idiomatic pattern in Swift, unlike the loop-and-add construction that had been required in Obj-C.The test bridging header is removed since there are no longer any
_Testheaders.Part of flutter/flutter#119108
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2